KV: introduce v2 error codes behind a CLI switch [KVL-1140] #11224

fabiotudone-da · 2021-10-13T05:35:09Z

Description

Using the same CLI switch as used by the participant, introduce v2 error codes for KV only when self-service error codes are enabled.

This is a sizable but extremely boring PR that is best reviewed commit-by-commit.

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
Normal production system change, include purpose of change in description
If you mean to change the status of a component, please make sure you keep the Component Status page up to date.

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

CHANGELOG_BEGIN CHANGELOG_END

fabiotudone-da · 2021-10-13T08:34:09Z

...ain/scala/com/daml/ledger/participant/state/kvutils/api/KeyValueParticipantStateReader.scala

+    logEntryToUpdate: (
+        DamlLogEntryId,
+        DamlLogEntry,
+        ValueSwitch[Status],
+        Option[Timestamp],
+    ) => List[Update],


This is not pretty but, considering the switch should be short-lived, I wouldn't re-architect (e.g. making it stateful) in this PR.

…uce-v2-codes

...e/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/KeyValueConsumption.scala

...src/main/scala/com/daml/ledger/participant/state/kvutils/updates/TransactionRejections.scala

...src/test/suite/scala/com/daml/ledger/participant/state/kvutils/KeyValueConsumptionSpec.scala

ghost

First pass.

...src/main/scala/com/daml/ledger/participant/state/kvutils/updates/TransactionRejections.scala

...kvutils/src/test/suite/scala/com/daml/ledger/participant/state/kvutils/ConversionsSpec.scala

...ant-state/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/Conversions.scala

nicu-da · 2021-10-14T14:45:53Z

...ant-state/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/Conversions.scala

 import com.daml.ledger.offset.Offset
 import com.daml.ledger.participant.state.kvutils.committer.transaction.Rejection
 import com.daml.ledger.participant.state.kvutils.committer.transaction.Rejection.{
  ExternallyInconsistentTransaction,
  InternallyInconsistentTransaction,
 }
+import com.daml.ledger.participant.state.kvutils.updates.TransactionRejections._


I think the idea is to try to use explicit imports

There is a bazillion of them so the IDE starified it; I am not sure, is our practice to always import them all explicitly?

That's my understanding from @SamirTalwar-DA . I updated my IJ settings to add an all import only after 99 imports of the same type.

...e/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/KeyValueConsumption.scala

nicu-da · 2021-10-14T14:52:46Z

...src/main/scala/com/daml/ledger/participant/state/kvutils/updates/TransactionRejections.scala

+  ExternallyInconsistentTransaction,
+  InternallyInconsistentTransaction,
+}
+import com.daml.ledger.participant.state.kvutils.store.events._


Ditto on the import

nicu-da · 2021-10-14T14:58:19Z

...src/test/suite/scala/com/daml/ledger/participant/state/kvutils/KeyValueConsumptionSpec.scala

+      forAll(
+        Table[ValueSwitch[Status]](
+          "Error Version",
+          v1ErrorSwitch,
+          v2ErrorSwitch,
+        )


This seems to be repeated a few times, can we reuse it instead of declaring it each time?

ghost

Very boring, but I am pretty happy with the changes.

Thank you for dealing with this.

fabiotudone-da · 2021-10-14T16:34:24Z

@meiersi-da I think you may want to review part of this PR in order to confirm the rejection -> error code mapping. Also @tudor-da FYI

…uce-v2-codes

miklos-da · 2021-10-18T09:32:03Z

...kvutils/src/test/suite/scala/com/daml/ledger/participant/state/kvutils/ConversionsSpec.scala

+  private lazy val v1ErrorSwitch = new ValueSwitch[Status](enableSelfServiceErrorCodes = false)
+  private lazy val v2ErrorSwitch = new ValueSwitch[Status](enableSelfServiceErrorCodes = true)


To improve readability, I'd suggest removing references to v1 or v2 as these are not self-explanatory:

Suggested change

private lazy val v1ErrorSwitch = new ValueSwitch[Status](enableSelfServiceErrorCodes = false)

private lazy val v2ErrorSwitch = new ValueSwitch[Status](enableSelfServiceErrorCodes = true)

private lazy val disabledSelfServiceErrorCodes = new ValueSwitch[Status](enableSelfServiceErrorCodes = false)

private lazy val enabledSelfServiceErrorCodes = new ValueSwitch[Status](enableSelfServiceErrorCodes = true)

miklos-da · 2021-10-19T13:13:25Z

...e/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/KeyValueConsumption.scala

    * @return [[Update]]s constructed from log entry.
    */
  // TODO(BH): add participantId to ensure participant id matches in DamlLogEntry
  @throws(classOf[Err])
  def logEntryToUpdate(
      entryId: DamlLogEntryId,
      entry: DamlLogEntry,
+      errorVersionSwitch: ValueSwitch[Status],


Can we have a default argument for these version switches on our public interfaces, please? I.e., the default should be that self-service error codes are enabled. We should only require specifying an explicit value when we would like to disable it.

@sofiafaro-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @sofiafaro-da is in charge of this release. Commit log: ``` 8ff347d Expand type synonyms lazily (#11282) 2fd200f KV: support contextual logging in KeyValueConsumption [KVL-1143] (#11288) f4df1cc converting server errors to proper client errors (#11184) 525e4ce Add GrpcStatus.toProto(JavaStatus) -> ScalaStatus conversion [KVL-1143] (#11292) 9b00a1a Rotate release rotation (#11291) dd09c38 Upgrade rules-nodejs (#11290) 87f1418 [Short] Remove unnecessary traits from ApiSubmissionServiceSpec (#11254) 6b65a72 Merge npm install steps in create-daml-app tests (#11287) a9f6afb kvutils: Rename `VersionedOffset` to `KVOffset`. (#11286) 98cf8d8 KV: introduce v2 error codes behind a CLI switch [KVL-1140] (#11224) 46f6877 Increase time model skew limits (#11273) 8f94cff kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffsetMutator`. [KVL-1154] (#11277) 81fde97 Bazel: Call `_wrap_rule` directly when building the Scala REPL rule. (#11279) edb2b04 Document scaladoc usage in Bazel (#11278) 6255837 Data dependencies support for reexports (#11147) 2fc7490 [Self-service error codes] Adapt error factories [DPP-656] (#11270) b1a6b11 ledger-api-test-tool: Add static time awareness [KVL-1156] (#11266) 243c120 Add an LF typechecking benchmark (#11276) 2db8c13 Test authorization within Exercise choice. (#11246) ec58ed6 Typecheck nested LF type applications more efficiently (#11253) a940016 Add debugRaw corresponding to traceM in Haskell (#11259) a988579 Fix Scala repl targets (#11272) e4808f6 Extract UtilLF module from daml-lf-conversion to its own library (#11263) 38712c0 sandbox-next - Disable participant command deduplication [KVL-1156] (#11264) 82fa229 Add daml-util-ghc lib (#11260) 68a93cf kvutils - Use the ledger configured time for command dedup [KVL-1149] (#11239) c8cede0 Bump startup time in create-daml-app tests (#11261) 4fac87f Bump the schema version for the JSON API (#11252) 5507670 update NOTICES file (#11256) 0063b10 Retry faster during startup (#11255) 04feb40 Improve reset service tests (#11240) b3375fd update compat versions for 1.18.0-snapshot.20211013.8071.0.514e8b50 (#11237) 9ed1eb3 Address security notice on `ansi-regex` (#11243) 387d0e8 Make compatibility migrations `eternal` (#11242) be899b3 Extract rejection_reason.proto from daml_kvutils.proto [KVL-1090] (#11235) a1d94e1 LF: Create special case class for VersionedContractInstance (#11127) b738988 Release 1.18 snapshot (#11234) 3c26852 LF: Use template Id in exercise node of fixed choice. (#11229) 139b6f3 CI: Set `PROJ_DIR` inside the bash lib, not outside. (#11236) ce64cb2 interfaces: Do some TODOs (#11231) 7a88c7d trigger-service: dev-mode-unsafe flag (#11233) 909a1bf [DPP-417][DDP-612] Adapt ApiSubmissionService to support V2 error codes (#11052) ``` Changelog: ``` - [JSON API] Several kinds of gRPC server errors are now reported with associated HTTP statuses; for example, a Daml-LF interpreter error now returns a 400 instead of a 500, and an exercise on an archived contract returns a 409 Conflict instead of a 500. Errors internal to JSON API (e.g. internal assertion failures) are no longer detailed in the HTTP response; their details are only logged. See `issue #11184 <https://github.com/digital-asset/daml/pull/11184>`__. - [Daml Stdlib] Add `debugRaw` as a convenience wrapper around `traceRaw` when used inside a do-block. `debugRaw` compares to `debug` like `traceRaw` compares to `trace` meaning it expects a `Text` instead of calling `show` on an expression. sandbox-next - Disable participant command deduplication kvutils - Command deduplication uses the configured time model (static/wall) and not running always using wall-clock [JSON API] Solving a bug that could cause the JSON API to return correct result if a contract with the same key is observed twice required a schema change. The JSON API data needs to be dropped and the query store needs to reset. If you are migrating from a previous version, either reset your database manually or start the HTTP JSON API with one of the options that regenerate the schema (`create-only`, `create-if-needed-and-start`, `create-and-start`). ``` CHANGELOG_BEGIN CHANGELOG_END

@sofiafaro-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @sofiafaro-da is in charge of this release. Commit log: ``` 8ff347d Expand type synonyms lazily (#11282) 2fd200f KV: support contextual logging in KeyValueConsumption [KVL-1143] (#11288) f4df1cc converting server errors to proper client errors (#11184) 525e4ce Add GrpcStatus.toProto(JavaStatus) -> ScalaStatus conversion [KVL-1143] (#11292) 9b00a1a Rotate release rotation (#11291) dd09c38 Upgrade rules-nodejs (#11290) 87f1418 [Short] Remove unnecessary traits from ApiSubmissionServiceSpec (#11254) 6b65a72 Merge npm install steps in create-daml-app tests (#11287) a9f6afb kvutils: Rename `VersionedOffset` to `KVOffset`. (#11286) 98cf8d8 KV: introduce v2 error codes behind a CLI switch [KVL-1140] (#11224) 46f6877 Increase time model skew limits (#11273) 8f94cff kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffsetMutator`. [KVL-1154] (#11277) 81fde97 Bazel: Call `_wrap_rule` directly when building the Scala REPL rule. (#11279) edb2b04 Document scaladoc usage in Bazel (#11278) 6255837 Data dependencies support for reexports (#11147) 2fc7490 [Self-service error codes] Adapt error factories [DPP-656] (#11270) b1a6b11 ledger-api-test-tool: Add static time awareness [KVL-1156] (#11266) 243c120 Add an LF typechecking benchmark (#11276) 2db8c13 Test authorization within Exercise choice. (#11246) ec58ed6 Typecheck nested LF type applications more efficiently (#11253) a940016 Add debugRaw corresponding to traceM in Haskell (#11259) a988579 Fix Scala repl targets (#11272) e4808f6 Extract UtilLF module from daml-lf-conversion to its own library (#11263) 38712c0 sandbox-next - Disable participant command deduplication [KVL-1156] (#11264) 82fa229 Add daml-util-ghc lib (#11260) 68a93cf kvutils - Use the ledger configured time for command dedup [KVL-1149] (#11239) c8cede0 Bump startup time in create-daml-app tests (#11261) 4fac87f Bump the schema version for the JSON API (#11252) 5507670 update NOTICES file (#11256) 0063b10 Retry faster during startup (#11255) 04feb40 Improve reset service tests (#11240) b3375fd update compat versions for 1.18.0-snapshot.20211013.8071.0.514e8b50 (#11237) 9ed1eb3 Address security notice on `ansi-regex` (#11243) 387d0e8 Make compatibility migrations `eternal` (#11242) be899b3 Extract rejection_reason.proto from daml_kvutils.proto [KVL-1090] (#11235) a1d94e1 LF: Create special case class for VersionedContractInstance (#11127) b738988 Release 1.18 snapshot (#11234) 3c26852 LF: Use template Id in exercise node of fixed choice. (#11229) 139b6f3 CI: Set `PROJ_DIR` inside the bash lib, not outside. (#11236) ce64cb2 interfaces: Do some TODOs (#11231) 7a88c7d trigger-service: dev-mode-unsafe flag (#11233) 909a1bf [DPP-417][DDP-612] Adapt ApiSubmissionService to support V2 error codes (#11052) ``` Changelog: ``` - [JSON API] Several kinds of gRPC server errors are now reported with associated HTTP statuses; for example, a Daml-LF interpreter error now returns a 400 instead of a 500, and an exercise on an archived contract returns a 409 Conflict instead of a 500. Errors internal to JSON API (e.g. internal assertion failures) are no longer detailed in the HTTP response; their details are only logged. See `issue #11184 <https://github.com/digital-asset/daml/pull/11184>`__. - [Daml Stdlib] Add `debugRaw` as a convenience wrapper around `traceRaw` when used inside a do-block. `debugRaw` compares to `debug` like `traceRaw` compares to `trace` meaning it expects a `Text` instead of calling `show` on an expression. sandbox-next - Disable participant command deduplication kvutils - Command deduplication uses the configured time model (static/wall) and not running always using wall-clock [JSON API] Solving a bug that could cause the JSON API to return correct result if a contract with the same key is observed twice required a schema change. The JSON API data needs to be dropped and the query store needs to reset. If you are migrating from a previous version, either reset your database manually or start the HTTP JSON API with one of the options that regenerate the schema (`create-only`, `create-if-needed-and-start`, `create-and-start`). ``` CHANGELOG_BEGIN CHANGELOG_END Co-authored-by: Azure Pipelines Daml Build <[email protected]>

Propagate error version switch to KeyValueConsumption

792afb3

CHANGELOG_BEGIN CHANGELOG_END

fabiotudone-da force-pushed the fabiotudone-da/self-service-errors/kv/introduce-v2-codes branch from 15b8817 to 792afb3 Compare October 13, 2021 08:29

fabiotudone-da commented Oct 13, 2021

View reviewed changes

Introduce v2 (self-service) KV error codes behind the CLI switch

ddcfe32

fabiotudone-da changed the title ~~KV: introduce new self-service error codes behind a cmdline switch [KVL-1140]~~ KV: introduce new self-service error codes behind a CLI switch [KVL-1140] Oct 13, 2021

Merge branch 'main' into fabiotudone-da/self-service-errors/kv/introd…

054a125

…uce-v2-codes

fabiotudone-da force-pushed the fabiotudone-da/self-service-errors/kv/introduce-v2-codes branch from 829cba4 to 054a125 Compare October 13, 2021 14:57

Test v2 codes and fix them

f3c7475

fabiotudone-da marked this pull request as ready for review October 13, 2021 16:34

fabiotudone-da requested review from a team as code owners October 13, 2021 16:34

fabiotudone-da changed the title ~~KV: introduce new self-service error codes behind a CLI switch [KVL-1140]~~ KV: introduce v2 error codes behind a CLI switch [KVL-1140] Oct 13, 2021

Merge branch 'main' into fabiotudone-da/self-service-errors/kv/introd…

8979128

…uce-v2-codes

fabiotudone-da force-pushed the fabiotudone-da/self-service-errors/kv/introduce-v2-codes branch from ee6fb61 to 8979128 Compare October 14, 2021 08:34

Merge branch 'main' into fabiotudone-da/self-service-errors/kv/introd…

013fd21

…uce-v2-codes

hubert-da approved these changes Oct 14, 2021

View reviewed changes

ghost reviewed Oct 14, 2021

View reviewed changes

...src/main/scala/com/daml/ledger/participant/state/kvutils/updates/TransactionRejections.scala Show resolved Hide resolved

...kvutils/src/test/suite/scala/com/daml/ledger/participant/state/kvutils/ConversionsSpec.scala Outdated Show resolved Hide resolved

nicu-da reviewed Oct 14, 2021

View reviewed changes

fabiotudone-da added 7 commits October 14, 2021 17:17

Keep newline separating methods in KeyValueConsumption

15b09f4

TransactionRejections: don't wrap updates in Some

91a0998

Factor errorVersionsTable

21ed5ca

Reorder imports

ebb21e6

Split "convert rejection to proto models and back to expected grpc code"

4b4861e

Remove unneeded Option returned by decodeTransactionRejectionEntry

7537e93

Formatting fix

8a067bf

ghost approved these changes Oct 14, 2021

View reviewed changes

Fix 7537e93

c39b990

fabiotudone-da requested review from meiersi-da and tudor-da October 14, 2021 16:34

nicu-da approved these changes Oct 15, 2021

View reviewed changes

tudor-da approved these changes Oct 18, 2021

View reviewed changes

Merge branch 'main' into fabiotudone-da/self-service-errors/kv/introd…

bbfaa71

…uce-v2-codes

fabiotudone-da enabled auto-merge (squash) October 18, 2021 15:55

Merge branch 'main' into fabiotudone-da/self-service-errors/kv/introd…

ca137e2

…uce-v2-codes

fabiotudone-da merged commit 98cf8d8 into main Oct 19, 2021

fabiotudone-da deleted the fabiotudone-da/self-service-errors/kv/introduce-v2-codes branch October 19, 2021 08:32

miklos-da reviewed Oct 19, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KV: introduce v2 error codes behind a CLI switch [KVL-1140] #11224

KV: introduce v2 error codes behind a CLI switch [KVL-1140] #11224

fabiotudone-da commented Oct 13, 2021 •

edited

Loading

fabiotudone-da Oct 13, 2021 •

edited

Loading

ghost left a comment

nicu-da Oct 14, 2021

fabiotudone-da Oct 14, 2021 •

edited

Loading

nicu-da Oct 14, 2021

nicu-da Oct 14, 2021

nicu-da Oct 14, 2021

fabiotudone-da Oct 14, 2021

ghost left a comment

fabiotudone-da commented Oct 14, 2021

miklos-da Oct 18, 2021

miklos-da Oct 19, 2021

		private lazy val v1ErrorSwitch = new ValueSwitch[Status](enableSelfServiceErrorCodes = false)
		private lazy val v2ErrorSwitch = new ValueSwitch[Status](enableSelfServiceErrorCodes = true)

KV: introduce v2 error codes behind a CLI switch [KVL-1140] #11224

KV: introduce v2 error codes behind a CLI switch [KVL-1140] #11224

Conversation

fabiotudone-da commented Oct 13, 2021 • edited Loading

Description

Pull Request Checklist

fabiotudone-da Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

nicu-da Oct 14, 2021

Choose a reason for hiding this comment

fabiotudone-da Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

nicu-da Oct 14, 2021

Choose a reason for hiding this comment

nicu-da Oct 14, 2021

Choose a reason for hiding this comment

nicu-da Oct 14, 2021

Choose a reason for hiding this comment

fabiotudone-da Oct 14, 2021

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

fabiotudone-da commented Oct 14, 2021

miklos-da Oct 18, 2021

Choose a reason for hiding this comment

miklos-da Oct 19, 2021

Choose a reason for hiding this comment

fabiotudone-da commented Oct 13, 2021 •

edited

Loading

fabiotudone-da Oct 13, 2021 •

edited

Loading

fabiotudone-da Oct 14, 2021 •

edited

Loading